[MGPU] App: default Kit renderer to one GPU - #5933
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 IsaacLab Review Bot — PR #5933
[MGPU] App: pin Kit renderer to single GPU under ISAACLAB_PIN_KIT_GPU
✅ Overall Assessment: LGTM (minor suggestions)
This is a clean, well-scoped, opt-in workaround for a documented upstream Kit bug. The implementation is minimal, reversible, and placed correctly.
📋 Summary of Changes
| File | Change |
|---|---|
source/isaaclab/isaaclab/app/app_launcher.py |
+19 lines: env var check + 3 Kit CLI flags + log message |
source/isaaclab/changelog.d/jichuanh-mgpu-pin-kit-resources.rst |
+14 lines: changelog fragment |
👍 Strengths
- Opt-in by default — Zero risk to existing single-GPU or user-facing paths. The env var must be explicitly set.
- Correct placement — The pinning logic sits in
_resolve_device_settings()right afteractive_gpu/physics_gpuassignment, which is the logical place for renderer GPU configuration. - Robust truthiness parsing — The
not in {"", "0", "false", "no", "off"}pattern after.lower()covers common shell conventions. - Good logging —
logger.info(...)emits a clear signal when the pin is active. - Thorough PR description — Excellent traceability to upstream bugs (NVBug 5687364, #3475) and documented WAR source (Kelly Guo).
- Changelog present — Properly formatted RST fragment.
💡 Minor Suggestions (non-blocking)
-
Idempotency guard: If a user already passes
--/renderer/multiGpu/enabled=Falsemanually via CLI, this would append a duplicate. Consider checkingsys.argvfor existing overrides before appending:if not any("--/renderer/multiGpu/enabled" in arg for arg in sys.argv): sys.argv.append("--/renderer/multiGpu/enabled=False")
(Low priority — Kit likely handles duplicates gracefully by last-wins, but explicit > implicit.)
-
Consider logging the trigger source: The log message could include the env var value for debugging:
logger.info("ISAACLAB_PIN_KIT_GPU=%s: pinning Kit renderer to a single GPU", raw_val)
-
Test coverage: The PR description mentions smoke testing locally, but there's no unit test for the env var parsing logic. A small parametrized test (mocking
os.environand checkingsys.argvafter_resolve_device_settings) would be valuable for regression, especially since this interacts with CI infrastructure.
🔍 Potential Concerns
- CI validation pending — The PR body notes that the actual A/B test against the multi-GPU hang is still pending (cherry-pick onto #5875). This is appropriate for merge given it's opt-in, but the CI experiment should be tracked to confirm efficacy.
CI Status
- ✅ pre-commit, labeler, changelog check, build wheel: pass
- ⏳ Installation tests, Docker builds, docs: pending (expected for fresh push)
Verdict: Ship it. Clean opt-in workaround with proper documentation. The minor suggestions above are non-blocking improvements that could be addressed in a follow-up.
🔄 Update (ce2ee35): Incremental review of new commit.
Changes Since 6d6a141
Single file modified: source/isaaclab/isaaclab/app/app_launcher.py
What changed:
- Added
--/physics/fabricUseGPUInterop=falseto the Kit CLI flags appended whenISAACLAB_PIN_KIT_GPUis set - Added a clear code comment explaining the rationale: the renderer multiGpu flags mitigate the startup-time enumeration race, while this new flag mitigates the runtime GPU-interop race
- Updated the log message to reflect the additional flag: now mentions "disabling fabric GPU-interop"
Assessment
✅ LGTM — This is a sensible hardening of the existing workaround:
- Correct layering — The original 3 flags prevented Kit's renderer from enumerating multiple GPUs at startup. This new flag addresses a separate runtime code path where Fabric's GPU-interop can still trigger cross-GPU issues.
- Safe for CI — The comment explicitly notes this is safe for the multi-GPU CI lane since those tests cover physics/scene/utility, not rendering.
- Minimal & focused — One additional CLI flag + updated log message. No behavioral change for users who don't set the env var.
- Good documentation — The inline comment clearly explains why both mitigations are needed (startup race vs runtime race).
No concerns with this increment. The original review's minor suggestions (idempotency guard, test coverage) still apply but remain non-blocking.
🔄 Update (22bf040): Branch rebased onto develop. The PR-specific changes (squashed into a single commit) are functionally identical to what was previously reviewed — all 4 Kit CLI flags + fabric GPU-interop disable + the changelog fragment. The large diff between SHAs is entirely from the rebase incorporating 22 other merged commits on develop. No new issues. Previous non-blocking suggestions (idempotency guard, test coverage) remain applicable but are not blocking.
🔄 Update (8ad9cd7): Incremental review of latest push.
Changes Since 22bf040
| File | Change |
|---|---|
source/isaaclab/isaaclab/app/app_launcher.py |
Refactored inline pinning logic into module-level helper |
source/isaaclab/test/app/test_env_var_launch.py |
Added parametrized unit tests for env var parsing |
What changed:
-
Refactored into testable helper — The env var parsing + CLI arg assembly is now a standalone function
_kit_single_gpu_pin_args(env_value)at module level, with:_KIT_SINGLE_GPU_PIN_ARGS: tuple constant holding the 4 Kit CLI flags_PIN_KIT_GPU_FALSY: frozenset of falsy string values- Clean docstring with Args/Returns sections
-
Unit tests added — Two
@pytest.mark.parametrizetests covering truthy values ("1","true","TRUE","yes","on","True") and falsy values (None,"","0","false","no","off","FALSE").
Assessment
✅ LGTM — This is an excellent improvement:
- Addresses previous test coverage suggestion — My original review noted the lack of unit tests for the env var parsing logic. This commit directly resolves that concern with well-structured parametrized tests.
- Better code organization — Extracting the logic into a pure function with constants makes it testable, reusable, and easier to reason about. The call site in
_resolve_device_settings()is now a clean two-liner. - Correct behavior — The
(env_value or "0")pattern correctly handlesNone(unset env var) by falling through to the falsy set. - No functional regression — The actual Kit CLI flags and activation logic are unchanged; this is purely a structural refactor.
All previous non-blocking suggestions are now resolved:
Test coverage✅ Addressed in this commit- Idempotency guard — remains a nice-to-have but is non-blocking (Kit handles duplicates via last-wins)
- Logging trigger source — minor, non-blocking
Final verdict: Ready to merge. Clean, well-tested, opt-in workaround.
Per per-PR minimum-needed analysis: - isaac-sim#5886 (bounded shutdown) is closed (audit verdict nice-to-have; isaac-sim#5933 prevents the hang upstream so the force-exit timer is moot). Reverts SIGHUP handler + ISAACLAB_FORCE_EXIT_TIMEOUT timer in AppLauncher; drops the workflow env var. - isaac-sim#5883 (kitless newton) kept open as a separate PR but left out of this diagnostic bundle to test whether isaac-sim#5933 alone is enough for newton test_articulation (which calls build_simulation_context(sim_cfg=, device=) at line 2427, so still needs isaac-sim#5881 for the cross-device kwarg fix). Reverts the newton test_articulation kitless conversion and the schemas.py _create_fixed_joint_to_world helper. Bundle now contains: isaac-sim#5823 + isaac-sim#5875 base + isaac-sim#5881 + isaac-sim#5933 + the JUnit XML path-collision fix in conftest. If green, confirms only 4 PRs are needed for multi-GPU CI green (with test_articulation un-gated).
Add an ISAACLAB_PIN_KIT_GPU env var to AppLauncher. When truthy, it appends Kit command-line overrides that pin the renderer to a single GPU (renderer.multiGpu.enabled=False, autoEnable=False, maxGpuCount=1) and disable the fabric GPU-interop path (physics.fabricUseGPUInterop= false), so each Kit process touches only its assigned GPU instead of enumerating every visible GPU at startup. Used by the multi-GPU CI workflow to avoid a shared GPU-interop context across concurrent sibling shards, which otherwise surfaces as "Stage X already attached" errors and SimulationApp.close hangs (see isaac-sim#3475). Off by default; single-GPU and user-facing rendering paths are unchanged.
ce2ee35 to
22bf040
Compare
8ad9cd7 to
22bf040
Compare
Greptile SummaryIntroduces an opt-in
Confidence Score: 4/5Safe to merge; the change is off by default and only activates when an explicit env var is set, leaving all existing code paths unchanged. The injection logic is straightforward and correctly placed before SimulationApp boots. The only non-trivial concern is that repeated AppLauncher construction in the same process would keep appending the same four flags to sys.argv, but since Kit respects last-value-wins for duplicates this does not alter runtime behaviour. The new env var is also undocumented relative to the rest of the class's env-var surface, but neither issue affects correctness of the primary use case. source/isaaclab/isaaclab/app/app_launcher.py — specifically the block that appends to sys.argv inside _resolve_device_settings. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User/CI
participant AL as AppLauncher.__init__
participant RDS as _resolve_device_settings
participant SA as SimulationApp
User->>AL: AppLauncher(launcher_args)
AL->>RDS: _resolve_device_settings(launcher_args)
RDS->>RDS: set active_gpu / physics_gpu
alt ISAACLAB_PIN_KIT_GPU is truthy
RDS->>RDS: sys.argv.append renderer multiGpu disabled
RDS->>RDS: "sys.argv.append fabricUseGPUInterop=false"
RDS->>RDS: logger.info pinning message
end
RDS-->>AL: launcher_args updated
AL->>SA: SimulationApp(_sim_app_config)
Note over SA: Kit reads sys.argv and restricts renderer to 1 GPU
Reviews (1): Last reviewed commit: "Add ISAACLAB_PIN_KIT_GPU to pin Kit rend..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if os.environ.get("ISAACLAB_PIN_KIT_GPU", "0").lower() not in {"", "0", "false", "no", "off"}: | ||
| sys.argv.append("--/renderer/multiGpu/enabled=False") | ||
| sys.argv.append("--/renderer/multiGpu/autoEnable=False") | ||
| sys.argv.append("--/renderer/multiGpu/maxGpuCount=1") | ||
| # Also disable the fabric GPU-interop path. The renderer multiGpu | ||
| # flags above mitigate the startup-time enumeration race; this | ||
| # mitigates the runtime GPU-interop race on top. Safe for the | ||
| # multi-GPU CI lane: it covers physics / scene / utility tests, | ||
| # not rendering. | ||
| sys.argv.append("--/physics/fabricUseGPUInterop=false") | ||
| logger.info( | ||
| "ISAACLAB_PIN_KIT_GPU enabled: pinning Kit renderer to a single GPU + disabling fabric GPU-interop" | ||
| ) |
There was a problem hiding this comment.
AppLauncher instantiation
If ISAACLAB_PIN_KIT_GPU is truthy and anything creates a second AppLauncher in the same process (e.g. test teardown/re-init), sys.argv will accumulate duplicate --/renderer/multiGpu/enabled=False entries on each call. Kit typically honours the last value, so functional behaviour is unaffected, but the argv list grows unboundedly. A guard like if not any("renderer/multiGpu/enabled" in a for a in sys.argv) before each append, or a single guard around the whole block, would prevent this.
| # https://github.com/isaac-sim/IsaacLab/issues/3475). The mitigation is | ||
| # to set ``renderer.multiGpu.enabled = false`` + ``maxGpuCount = 1`` so | ||
| # each Kit only touches its assigned GPU. | ||
| if os.environ.get("ISAACLAB_PIN_KIT_GPU", "0").lower() not in {"", "0", "false", "no", "off"}: |
There was a problem hiding this comment.
ISAACLAB_PIN_KIT_GPU not documented in the class docstring
All other AppLauncher-consumed environment variables (e.g. LIVESTREAM, HEADLESS) are listed in the class-level docstring and/or _APPLAUNCHER_CFG_INFO. ISAACLAB_PIN_KIT_GPU is only referenced in an inline comment, making it invisible to users reading the API docs. Adding a short entry to the class docstring (or a .. envvar:: note in the _resolve_device_settings docstring) would keep discoverability consistent with the rest of the env-var surface.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # https://github.com/isaac-sim/IsaacLab/issues/3475). The mitigation is | ||
| # to set ``renderer.multiGpu.enabled = false`` + ``maxGpuCount = 1`` so | ||
| # each Kit only touches its assigned GPU. | ||
| if os.environ.get("ISAACLAB_PIN_KIT_GPU", "0").lower() not in {"", "0", "false", "no", "off"}: |
There was a problem hiding this comment.
is there a good reason why we shouldn't always have these set? this feels like could be necessary for multi GPU training as well.
There was a problem hiding this comment.
Wouldn't that explicitely prevent multi-gpu rendering? But I agree with the default, maybe we should make this a default, and for people that want to do single process multi-gpu tell them to enable that flag?
There was a problem hiding this comment.
Or we could add a flag? Might be more visible than an environment variable?
There was a problem hiding this comment.
ya I think for our current workflows, having this as the default makes more sense
There was a problem hiding this comment.
I will test on if making this default will break anything
Disable single-process renderer multi-GPU by default and provide an explicit opt-in. Keep the Fabric interop mitigation independent so multi-GPU CI can apply it temporarily without changing user-facing renderer behavior.
hujc7
changed the title
| help='The device to run the simulation on. Can be "cpu", "cuda", "cuda:N", where N is the device ID', | ||
| ) | ||
| arg_group.add_argument( | ||
| "--multi_gpu", |
There was a problem hiding this comment.
I think this might be confusing to users since it's not the same multi GPU flag we have for training (--distributed). users should be able to always set this through the --kit_args flag already
| default_args = [] | ||
| if not launcher_args["multi_gpu"]: | ||
| default_args = [ | ||
| "--/renderer/multiGpu/enabled=false", |
There was a problem hiding this comment.
we can probably put default settings directly in the .kit files?
Default the Isaac Lab Kit experiences to one renderer GPU while keeping XR explicitly multi-GPU. Keep Fabric GPU interop independent through a temporary environment override so CI can disable it without changing renderer behavior.
hujc7
changed the title
Keep the single-GPU renderer limit in the base Kit experiences and let the rendering variants inherit it alongside the other renderer multi-GPU defaults.
| # import logger | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| _FABRIC_GPU_INTEROP_ENV = "ISAACLAB_FABRIC_USE_GPU_INTEROP" |
There was a problem hiding this comment.
were we able to figure out why this was needed for CI? ideally I think this flag should always be on.
There was a problem hiding this comment.
Mainly the performance impact (agent claiming ~5% but needs to be confirmed). The problem currently only repros on CI instances when running tests in parallel, which makes it hard to investigate. The goal is to eventually remove those so currently it's setup as a WAR.
There was a problem hiding this comment.
ok let's create a ticket before merging this so that we keep track of the issue and remind us to remove this workaround.
The renderer-pinning env var ISAACLAB_PIN_KIT_GPU was superseded by isaac-sim#5933, which makes single-GPU rendering the Kit default and adds ISAACLAB_FABRIC_USE_GPU_INTEROP for the fabric GPU-interop override. Point the multi-GPU CI launcher/runner at the new env var.
Reconcile against the split-out changes that have since merged: - Drop the renderer-pinning ISAACLAB_PIN_KIT_GPU block and its changelog fragment; superseded by isaac-sim#5933 (default one-GPU + ISAACLAB_FABRIC_USE_GPU_INTEROP). - Drop the inline newton_manager.py torch/Warp device pins and take develop's version; superseded by isaac-sim#6322 (centralized set_cuda_device + ScopedCapture(device=device)). - Resolve test_views_xform_prim_fabric.py: keep develop's isaac-sim#5677/isaac-sim#5380 test bodies, retain the device_scope test_devices() parametrization.
…overage (#5823) ## 1. Summary - **CI:** Adds a dedicated multi-GPU pytest workflow that runs one shard per non-default `cuda:N`, with a shared atomic work queue, per-file reports, and end-of-run reconciliation. - **isaaclab:** Adds composable `DeviceScope` flags and `test_devices()` for `scope ∩ runtime` parametrization while retaining exact custom string masks. - **isaaclab:** Uses `ISAACLAB_TEST_DEVICES` as the single source for both test parametrization and explicit Kit launch-device selection; `AppLauncher` has no implicit test-environment override. - **isaaclab:** Migrates 19 device-parametrized test modules across the core, Newton, OV PhysX, and PhysX suites while preserving the existing cpu + cuda:0 single-GPU behavior. ## 2. Device selection ```python from isaaclab.test.utils import DeviceScope, test_devices test_devices() # cpu + every GPU test_devices(DeviceScope.CUDA) # every GPU test_devices(DeviceScope.CPU | DeviceScope.NON_DEFAULT_CUDA) # composed scope test_devices("101X") # exact custom mask ``` - `ISAACLAB_TEST_DEVICES` limits which devices a run may use; unset preserves the historical cpu + cuda:0 runtime. - The multi-GPU lane discovers non-default-capable scopes and narrows each shard to one concrete device. ## 3. Dependencies All prerequisite changes have merged into `develop`, so this PR no longer carries them — it is now scoped to the device-selection infrastructure and the multi-GPU pytest workflow only: - #5695 — Cross-platform Part 1 (base pytest markers / shared scaffolding). - #5881 — Sim honors the device kwarg over `sim_cfg.device` in `build_simulation_context`. - #5933 — Kit renderer defaults to one GPU + adds `ISAACLAB_FABRIC_USE_GPU_INTEROP`; the multi-GPU lane sets `ISAACLAB_FABRIC_USE_GPU_INTEROP=0`. - #6322 — Newton multi-GPU initialization on non-default CUDA devices (`cuda:1` and higher). ## 4. Test plan - [x] Device-selection unit suite passes. - [x] Changed Python modules compile; multi-GPU shell scripts pass `bash -n`. - [x] Full documentation build (`./isaaclab.sh -d`). - [x] Full pre-commit suite (`./isaaclab.sh -f`). - [x] GitHub Actions green on latest `develop` (one `rendering-correctness-kitless` flake cleared on rerun; fixed upstream on `develop` by #6431).
3 participants
1. Summary
kit_args, avoiding overlap with training--distributed.ISAACLAB_FABRIC_USE_GPU_INTEROP=0|1as an independent temporary CI workaround; when unset, the Kit default is preserved.2. Opt-in and workaround
Enable single-process renderer multi-GPU explicitly:
The multi-GPU CI lane can temporarily disable Fabric GPU interop with
ISAACLAB_FABRIC_USE_GPU_INTEROP=0. Explicit raw Kit settings andkit_argstake precedence over the environment override. Remove the workaround after the underlying Kit/PhysX problem is fixed.3. Validation
developwithout conflicts../isaaclab.sh -fpassed after the merge.